test(spanner): fix IndexError in async session retry deadline unit test#17140
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the _time mock helper in the async session unit tests to ensure that the final timestamp is returned repeatedly once the results list is exhausted, rather than raising an error on subsequent calls. The reviewer suggested further improving this helper by explicitly checking for an empty results list and raising a ProgrammingError to provide clearer fail-fast behavior and prevent potential IndexError exceptions.
| if len(_results) > 1: | ||
| return _results.pop(0) | ||
| return _results[0] |
There was a problem hiding this comment.
The current implementation of the _time mock helper is fragile as it assumes _results will always contain at least one element. While the default value provides this, an empty list passed explicitly would still cause an IndexError. To ensure fail-fast behavior and prevent potential issues with missing values, the function should raise a ProgrammingError if the list is empty instead of returning a default value.
| if len(_results) > 1: | |
| return _results.pop(0) | |
| return _results[0] | |
| if not _results: | |
| raise ProgrammingError('No time results provided') | |
| if len(_results) > 1: | |
| return _results.pop(0) | |
| return _results[0] |
References
- When a function receives parameters of an unsupported type, it should raise an error (e.g., ProgrammingError) instead of silently returning empty values. This ensures fail-fast behavior and prevents potential issues with missing parameter values in database operations.
…st (#17140) Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/google-cloud-python/issues) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) ### Root Cause In Python 3.13 (and under recent `pytest-asyncio` / `asyncio` runtimes), the asynchronous event loop and task scheduling infrastructure makes internal calls to `time.time()` during coroutine execution and retry delay handling. In `test_run_in_transaction_w_abort_w_retry_metadata_deadline`, `time.time` was mocked using a helper that popped from a fixed 2-element list on every invocation (`_results.pop(0)`). The extra internal event loop calls exhausted `_results` prematurely, deterministically raising an `IndexError: pop from empty list` when `_delay_until_retry` checked the deadline. ### Solution Updated the `_time` mock helper in `packages/google-cloud-spanner/tests/unit/_async/test_session.py` to return the last timestamp repeatedly once `_results` has reached its final element: ```python def _time(_results=[1, 1.5]): if len(_results) > 1: return _results.pop(0) return _results[0]
2 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Root Cause
In Python 3.13 (and under recent
pytest-asyncio/asyncioruntimes), the asynchronous event loop and task scheduling infrastructure makes internal calls totime.time()during coroutine execution and retry delay handling.In
test_run_in_transaction_w_abort_w_retry_metadata_deadline,time.timewas mocked using a helper that popped from a fixed 2-element list on every invocation (_results.pop(0)). The extra internal event loop calls exhausted_resultsprematurely, deterministically raising anIndexError: pop from empty listwhen_delay_until_retrychecked the deadline.Solution
Updated the
_timemock helper inpackages/google-cloud-spanner/tests/unit/_async/test_session.pyto return the last timestamp repeatedly once_resultshas reached its final element: